Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: disable button while file is being downloaded #129

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

javier-godoy
Copy link
Member

@javier-godoy javier-godoy commented Jun 11, 2024

#126

Summary by CodeRabbit

  • New Features

    • Enhanced document export functionality with support for concurrent downloads.
    • Added callback handling during the download process for improved user feedback.
    • Introduced a control to prevent initiating multiple downloads simultaneously.
  • Bug Fixes

    • Improved management of download state to prevent overlapping download requests.
  • Tests

    • Updated tests to cover new concurrent download features and event handling.

Copy link

coderabbitai bot commented Jun 11, 2024

Walkthrough

The recent updates introduce new callback mechanisms and state management to the ConcurrentStreamResourceWriter class, enhancing its handling of concurrent downloads. The GridExporter class has been updated to manage document export streams more effectively, including a new boolean field to control simultaneous download requests. Additionally, adjustments in service initialization prevent multiple downloads from being initiated concurrently. These changes collectively improve the robustness and user experience of the document export functionality.

Changes

Files Change Summary
src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java Introduced an AtomicBoolean field for download state, added abstract methods onAccept() and onFinish() for handling callbacks during the download process, and included boolean fields for tracking state.
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java Added new imports, modified method signatures for document export resources to return GridExporterStreamResource, and introduced a boolean field disableOnClick to manage concurrent download requests.
src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java Added a call to GridExporter.setDisableOnClick(true) in the serviceInit method to prevent additional downloads while one is in progress.
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java Added boolean fields (accepted, finished) and overridden methods (onAccept(), onFinish()) in the ConcurrentStreamResourceWriter class to manage acceptance and completion states.
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java Set the disableOnClick property of the exporter object to true to prevent additional download requests during ongoing downloads.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant G as GridExporter
    participant C as ConcurrentStreamResourceWriter
    participant D as Document

    U->>G: Request document export
    G->>C: Initialize export process
    C->>D: Generate document stream
    D-->>C: Return document stream
    C->>C: onAccept() callback
    C->>U: Provide document stream
    U->>C: Acknowledge download completion
    C->>C: onFinish() callback
Loading

Poem

In the world of code so bright,
New features come to light.
With streams that flow and callbacks that show,
Exports now with a smoother glow.
GridExporter stands tall and neat,
Making downloads quite the treat.
🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@javier-godoy
Copy link
Member Author

@coderabbitai review

Copy link

coderabbitai bot commented Jun 11, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Outside diff range and nitpick comments (2)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2)

Line range hint 41-62: Refactor test methods to improve clarity and maintainability.

Consider breaking down complex test methods into smaller, more focused tests. This can improve readability and make it easier to identify the specific conditions each test is verifying.


Line range hint 41-62: Optimize synchronization mechanisms in test setup.

Evaluate the use of CountDownLatch and CyclicBarrier for efficiency and correctness. Ensure that they are used in a way that accurately simulates the intended concurrency scenarios.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 17576de and 4134683.

Files selected for processing (3)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (4 hunks)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (4 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2 hunks)
Additional context used
Learnings (2)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (1)
User: javier-godoy
PR: FlowingCode/GridExporterAddon#125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
User: javier-godoy
PR: FlowingCode/GridExporterAddon#125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.

Comment on lines +41 to +42
private boolean accepted;
private boolean finished;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper encapsulation of state management in ConcurrentStreamResourceWriter.

Consider making the boolean fields private to ensure they are only modified within the class itself, enhancing encapsulation and reducing the risk of unintended side effects from external modifications.

Also applies to: 53-62

Comment on lines +163 to +189
/**
* Callback method that is invoked when a download is accepted.
* <p>
* This method is called at the start of the download process, right after the
* {@link #accept(OutputStream, VaadinSession) accept} method is invoked and it has been
* determined that the download can proceed. Subclasses should implement this method to perform
* any necessary actions before the download begins, such as initializing resources, logging, or
* updating the UI to reflect the start of the download.
* <p>
* Note that this method is called before any semaphore permits are acquired, so it is executed
* regardless of whether the semaphore is enabled or not.
* </p>
*/
protected abstract void onAccept();

/**
* Callback method that is invoked when a download finishes.
* <p>
* This method is called at the end of the download process, right before the
* {@link #accept(OutputStream, VaadinSession) accept} method returns, regardless of whether the
* download was successful, timed out, or encountered an error. Subclasses should implement this
* method to perform any necessary actions after the download completes, such as releasing
* resources, logging, or updating the UI to reflect the completion of the download.
* <p>
* Note that this method is always called, even if an exception is thrown during the download
* process, ensuring that any necessary cleanup can be performed.
* </p>
*/
protected abstract void onFinish();

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure correct implementation of callback methods in ConcurrentStreamResourceWriter.

Verify that the onAccept, onFinish, and onTimeout methods are implemented in a way that they correctly handle their respective stages of the download process. These methods should allow for clean separation of concerns and easy maintenance.

Comment on lines 211 to 238
if (!downloading.compareAndSet(false, true)) {
throw new IllegalStateException();
}

if (!enabled) {
delegate.accept(stream, session);
} else {
try {
onAccept();
if (!enabled) {
delegate.accept(stream, session);
} else {

try {
try {

int permits;
float cost = getCost(session);
synchronized (semaphore) {
permits = costToPermits(cost, semaphore.maxPermits);
}
int permits;
float cost = getCost(session);
synchronized (semaphore) {
permits = costToPermits(cost, semaphore.maxPermits);
}

if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) {
try {
delegate.accept(stream, session);
} finally {
semaphore.release(permits);
if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) {
try {
delegate.accept(stream, session);
} finally {
semaphore.release(permits);
}
} else {
onTimeout();
throw new InterruptedByTimeoutException();
}
} else {
onTimeout();
throw new InterruptedByTimeoutException();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw (IOException) new InterruptedIOException().initCause(e);
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw (IOException) new InterruptedIOException().initCause(e);
}
} finally {
downloading.set(false);
onFinish();
Copy link

@coderabbitai coderabbitai bot Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review semaphore logic in the accept method for correctness and robustness.

Ensure that the semaphore logic within the accept method is robust and correctly handles different scenarios such as timeouts and interruptions. The method should fail gracefully in case of errors and ensure resources are always released.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai when would the implementation fail to release resources?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javier-godoy, the implementation in the accept method of ConcurrentStreamResourceWriter.java is designed to ensure that resources are always released, regardless of how the method exits. Here's how it handles different scenarios:

  1. Semaphore Permits: The semaphore permits are acquired conditionally within a nested try block. Regardless of the outcome (successful acquisition or timeout), the permits are released in the finally block associated with this nested try. This ensures that if permits are acquired, they are always released, even if an exception like InterruptedByTimeoutException is thrown.

  2. AtomicBoolean downloading: This flag is set to true at the beginning of the method and is always set back to false in the outer finally block. This ensures that the flag is reset even if an exception occurs during the download process.

  3. Exception Handling: If an InterruptedException occurs, it is caught, and the thread's interrupt status is set again with Thread.currentThread().interrupt(), followed by throwing an InterruptedIOException. This is done inside the inner try block, ensuring that all resources are still released properly in the finally blocks.

The method is structured to fail gracefully by ensuring that all resources (semaphore permits and the downloading flag) are cleaned up in the finally blocks, which are always executed regardless of whether an exception occurs. This robust design prevents resource leakage under normal operation conditions. If you have specific scenarios in mind where you suspect resources might not be released, please share them, and I can look into those specifically.

Comment on lines +296 to +326
public GridExporterStreamResource getDocxStreamResource() {
return getDocxStreamResource(null);
}

public StreamResource getDocxStreamResource(String template) {
return new StreamResource(fileName + ".docx",
public GridExporterStreamResource getDocxStreamResource(String template) {
return new GridExporterStreamResource(fileName + ".docx",
makeConcurrentWriter(new DocxStreamResourceWriter<>(this, template)));
}

public StreamResource getPdfStreamResource() {
public GridExporterStreamResource getPdfStreamResource() {
return getPdfStreamResource(null);
}

public StreamResource getPdfStreamResource(String template) {
return new StreamResource(fileName + ".pdf",
public GridExporterStreamResource getPdfStreamResource(String template) {
return new GridExporterStreamResource(fileName + ".pdf",
makeConcurrentWriter(new PdfStreamResourceWriter<>(this, template)));
}

public StreamResource getCsvStreamResource() {
return new StreamResource(fileName + ".csv", new CsvStreamResourceWriter<>(this));
}

public StreamResource getExcelStreamResource() {
public GridExporterStreamResource getExcelStreamResource() {
return getExcelStreamResource(null);
}

public StreamResource getExcelStreamResource(String template) {
return new StreamResource(fileName + ".xlsx",
public GridExporterStreamResource getExcelStreamResource(String template) {
return new GridExporterStreamResource(fileName + ".xlsx",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review new methods for generating stream resources in GridExporter.

Ensure that the methods getDocxStreamResource, getPdfStreamResource, and getExcelStreamResource are correctly implemented. They should integrate seamlessly with the rest of the application and handle all edge cases.

Comment on lines +154 to +175
excelLink
.setHref(exporter.getExcelStreamResource(excelCustomTemplate)
.forComponent(excelLink));
excelLink.getElement().setAttribute("download", true);
footerToolbar.add(
new FooterToolbarItem(excelLink, FooterToolbarItemPosition.EXPORT_BUTTON));
}
if (exporter.isDocxExportEnabled()) {
Anchor docLink = new Anchor("", FontAwesome.Regular.FILE_WORD.create());
docLink.setHref(exporter.getDocxStreamResource(docxCustomTemplate));
docLink.setHref(
exporter.getDocxStreamResource(docxCustomTemplate).forComponent(docLink));
docLink.getElement().setAttribute("download", true);
footerToolbar
.add(new FooterToolbarItem(docLink, FooterToolbarItemPosition.EXPORT_BUTTON));
}
if (exporter.isPdfExportEnabled()) {
Anchor docLink = new Anchor("", FontAwesome.Regular.FILE_PDF.create());
docLink.setHref(exporter.getPdfStreamResource(docxCustomTemplate));
docLink.setHref(
exporter.getPdfStreamResource(docxCustomTemplate).forComponent(docLink));
Copy link

@coderabbitai coderabbitai bot Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the integration of UI components and concurrency settings in GridExporter.

Ensure that the integration of UI components with concurrency settings is handled correctly. The methods that configure and manage buttons should work seamlessly with the concurrency logic to provide a consistent user experience.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Please explain what you mean

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javier-godoy, I'd be happy to clarify the integration of UI components with concurrency settings in the GridExporter class.

In the GridExporter class, the integration primarily revolves around managing the UI components, specifically the buttons for exporting documents, in conjunction with the concurrency settings that control the download processes.

Here's a breakdown of how it works:

  1. UI Component Integration: Each export button (for Excel, DOCX, PDF) is linked to a corresponding GridExporterStreamResource via the forComponent() method. This method associates a button with a specific download action, ensuring that the download is triggered when the button is clicked.

  2. Concurrency Management: The GridExporterStreamResource uses an instance of GridExporterConcurrentStreamResourceWriter to write the stream. This class extends ConcurrentStreamResourceWriter and is crucial for managing concurrency. It implements methods like onAccept() and onFinish():

    • onAccept(): Called when a download is initiated. This method disables the associated button to prevent further clicks and thus additional concurrent downloads.
    • onFinish(): Called once the download is complete. This re-enables the button, allowing further actions.
  3. Concurrency Settings: These settings are configured to manage how downloads are handled when multiple requests are made. For instance, settings like concurrentDownloadCost and concurrentDownloadTimeoutNanos help in defining the cost of a download and how long the system should wait before timing out a download request, respectively.

The integration ensures that the user experience is smooth and consistent, preventing any issues that might arise from concurrent downloads, such as multiple downloads interfering with each other or overloading the server.

I hope this explanation helps! If you have any more questions or need further details, feel free to ask.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai You asked to "Review the integration of UI components and concurrency settings in GridExporter and ensure that the integration of UI components with concurrency settings is handled correctly" but according to your explanation, the integration seems correct.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javier-godoy, I'm glad to hear that the explanation met your expectations and that the integration of UI components with concurrency settings in the GridExporter class appears correct to you. If you have any more questions, need further clarifications, or if there's anything else I can assist you with, please don't hesitate to ask!

Base automatically changed from feat-concurrent to master June 18, 2024 16:46
Copy link

sonarcloud bot commented Jul 15, 2024

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4134683 and d86c29d.

Files selected for processing (4)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (2 hunks)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (6 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2 hunks)
Additional comments not posted (16)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1)

23-24: LGTM!

The addition of GridExporter.setDisableOnClick(true) effectively prevents additional downloads from starting while one is already in progress, improving control flow and user experience.

src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (3)

160-173: LGTM!

The addition of the onAccept method allows subclasses to perform necessary pre-download actions, enhancing the control flow and logic of the download process.


175-188: LGTM!

The addition of the onFinish method allows subclasses to perform necessary post-download actions, ensuring that essential actions are performed consistently.


208-238: LGTM!

The modifications to the accept method ensure that pre-download and post-download actions are executed consistently, improving the robustness of the download handling process.

src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2)

53-56: LGTM!

The overridden onAccept method correctly sets the accepted variable to true, allowing the class to track the acceptance state of the resource writer.


58-61: LGTM!

The overridden onFinish method correctly sets the finished variable to true, allowing the class to track the completion state of the resource writer.

src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (10)

24-26: LGTM!

The new imports for Component and HasEnabled are necessary for the new functionality.


86-86: LGTM!

The new boolean field disableOnClick is correctly added to control concurrent downloads.


299-300: LGTM!

The method getDocxStreamResource has been correctly updated to return GridExporterStreamResource.


303-304: LGTM!

The method getDocxStreamResource(String template) has been correctly updated to return GridExporterStreamResource.


308-309: LGTM!

The method getPdfStreamResource has been correctly updated to return GridExporterStreamResource.


312-313: LGTM!

The method getPdfStreamResource(String template) has been correctly updated to return GridExporterStreamResource.


321-322: LGTM!

The method getExcelStreamResource has been correctly updated to return GridExporterStreamResource.


325-326: LGTM!

The method getExcelStreamResource(String template) has been correctly updated to return GridExporterStreamResource.


330-333: LGTM!

The method makeConcurrentWriter has been correctly updated to return GridExporterConcurrentStreamResourceWriter.


349-395: LGTM!

The class GridExporterConcurrentStreamResourceWriter is correctly implemented to manage concurrent downloads and UI updates.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d86c29d and b60aa2e.

Files selected for processing (5)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (2 hunks)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (6 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2 hunks)
Files skipped from review due to trivial changes (1)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java
Files skipped from review as they are similar to previous changes (1)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
Additional context used
Learnings (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1)
Learnt from: javier-godoy
PR: FlowingCode/GridExporterAddon#125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Additional comments not posted (22)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1)

118-119: LGTM!

The change to set the disableOnClick property of the exporter object to true improves the robustness of the download process by preventing multiple concurrent downloads.

src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (10)

160-173: LGTM!

The onAccept method is correctly defined as an abstract method, allowing subclasses to implement necessary actions before the download begins.


175-188: LGTM!

The onFinish method is correctly defined as an abstract method, allowing subclasses to implement necessary actions after the download completes.


208-209: LGTM!

The call to onAccept ensures that any pre-download logic is executed before the semaphore permits are acquired.


237-238: LGTM!

The call to onFinish ensures that any post-download actions are executed regardless of whether an exception occurs during the download process.


Line range hint 374-377: LGTM!

The onTimeout method is correctly defined as an abstract method, allowing subclasses to implement necessary actions in response to the timeout.


229-230: LGTM!

The call to onTimeout ensures that any necessary actions in response to the timeout are executed.


208-209: LGTM!

The call to onAccept ensures that any pre-download logic is executed before the semaphore permits are acquired.


237-238: LGTM!

The call to onFinish ensures that any post-download actions are executed regardless of whether an exception occurs during the download process.


229-230: LGTM!

The call to onTimeout ensures that any necessary actions in response to the timeout are executed.


208-209: LGTM!

The calls to onAccept and onFinish ensure that any pre-download and post-download actions are executed correctly.

Also applies to: 237-238

src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (11)

86-86: LGTM!

The disableOnClick field is correctly defined and initialized.


299-299: LGTM!

The update to the getDocxStreamResource method signature is correctly applied and aligns with the new resource management strategy.


303-304: LGTM!

The update to the getDocxStreamResource(String template) method signature is correctly applied and aligns with the new resource management strategy.


308-308: LGTM!

The update to the getPdfStreamResource method signature is correctly applied and aligns with the new resource management strategy.


312-313: LGTM!

The update to the getPdfStreamResource(String template) method signature is correctly applied and aligns with the new resource management strategy.


321-321: LGTM!

The update to the getExcelStreamResource method signature is correctly applied and aligns with the new resource management strategy.


325-326: LGTM!

The update to the getExcelStreamResource(String template) method signature is correctly applied and aligns with the new resource management strategy.


330-333: LGTM!

The modification of the makeConcurrentWriter method to return GridExporterConcurrentStreamResourceWriter is correctly applied and aligns with the new resource management strategy.


335-347: LGTM!

The GridExporterStreamResource class is correctly defined and aligns with the new resource management strategy.


349-395: LGTM!

The GridExporterConcurrentStreamResourceWriter class is correctly defined and aligns with the new resource management strategy.


494-506: LGTM!

The setDisableOnClick method is correctly defined and aligns with the new resource management strategy.

@javier-godoy javier-godoy marked this pull request as draft August 29, 2024 19:48
@javier-godoy javier-godoy marked this pull request as ready for review August 29, 2024 19:53
@javier-godoy javier-godoy changed the title WIP: perf: prevent concurrent downloads for the same instance feat: disable button while file is being downloaded Aug 29, 2024
Copy link
Member

@mlopezFC mlopezFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this was not intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I reverted that change.

Copy link

sonarcloud bot commented Sep 4, 2024

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b60aa2e and 3e40dac.

Files selected for processing (4)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (2 hunks)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (6 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1 hunks)
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java
  • src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
Additional context used
Learnings (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1)
Learnt from: javier-godoy
PR: FlowingCode/GridExporterAddon#125
File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402
Timestamp: 2024-06-09T06:20:09.702Z
Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Additional comments not posted (5)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/GridExporterBigDatasetDemo.java (1)

118-119: LGTM!

The code changes align with the PR objective of disabling the button while a file is being downloaded. This enhancement prevents users from initiating multiple downloads simultaneously, thereby improving the user experience and increasing the robustness of the download process.

src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (4)

86-86: LGTM!

The addition of the disableOnClick field is approved. It aligns with the feature of preventing multiple concurrent downloads, as mentioned in the AI-generated summary.


330-333: LGTM!

The change to the makeConcurrentWriter method to return an instance of GridExporterConcurrentStreamResourceWriter is approved. It aligns with the introduction of the new class for handling concurrent download operations, as mentioned in the AI-generated summary.


335-396: LGTM!

The addition of the GridExporterStreamResource and GridExporterConcurrentStreamResourceWriter classes is approved. The classes align with the AI-generated summary and provide the necessary functionality for managing document export streams and handling concurrent download operations. The forComponent method in GridExporterStreamResource and the overridden methods in GridExporterConcurrentStreamResourceWriter enable button state management and concurrency control.


494-506: LGTM!

The addition of the setDisableOnClick method is approved. It provides a way to configure the behavior of the system when a download is in progress, aligning with the AI-generated summary and the addition of the disableOnClick field.

Copy link
Member

@mlopezFC mlopezFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mlopezFC mlopezFC merged commit 88922a1 into master Sep 10, 2024
3 checks passed
@mlopezFC mlopezFC deleted the feat-126 branch September 10, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants